ℹ️ To trigger the CI, please tick the checkbox below 👇

• Click to trigger kibana-pull-request for this PR!
• Click to trigger kibana-deploy-project-from-pr for this PR!

@albertoblaz, thanks for the comments!

I'm only concerned with parts of the state that used to be local and are now centralized here, which might cause additional re-renderings but I haven't done any testing. If you've tested it and works fine then ignore this :)

I have not seen a difference in re-renders, but using a context such as this where the main business logic for the page is encapsulated in the Context shows me where we can act on performance enhancements.

One such example, we are fetching ALL the rules for a specific benchmark on every render (AWS, Kubernetes, etc), we can make some changes there by using a useRef to ensure that all the rules are only fetched once.

The Rules page is complex in its current state because we have business logic in components that are dependent on the props that are passed to it, additionally, we have the functionality to update the status of the rules in multiple places. When we need to make changes to the rules page, it is hard to follow the logic because it is spread across 4 component files and their inner components.

I like to subscribe to the notion of avoiding hasty performance enhancements, until it is a real problem to solve.

Thanks for putting up the POC!
I mostly had experience using context for small atomic pieces of state required for components spread in the component tree far away from each other. In your example, it feels like the context is used not in the way it was intended to be used. Here is why I have this feeling:

• the context is huge, almost the whole state of the rules page is packed into one context
• the consumer components are not spread in the component tree, they are all on the same level under one container
  I don't have specific arguments for why it might bite us if we go with this approach, except the unnecessary rerenders which has already been brought up and the fact that for every future component even if it relies on only one piece of the state, it will be hard not to use this huge context

• the context is huge, almost the whole state of the rules page is packed into one context

@maxcold you are 100% right. This is a large context, however, this is mostly what the RulesContainer component is doing today with a few minor changes...

• I destructured the rulesQuery and rulesPageData so the context now looks larger
  • We were duplicating the pageSize attribute in both those objects
  • Objects are not good for state change validation
  • We are currently using useMemo for rulesQuery and rulesPageData; however with objects, we need to do a deep comparison with useMemo
  • The destructured objects attributes are mostly arrays and primitive types

the consumer components are not spread in the component tree, they are all on the same level under one container
I don't have specific arguments for why it might bite us if we go with this approach, except the unnecessary rerenders

When you say rerender, are we talking about rendering a virtual DOM or the page? I do not see a difference in page rendering between this proposal and what is currently in use. This change proposal is more about the readability and maintainability of the page(s). When we hastily address performance before there is a performance problem, we are adding more code that JS needs to parse and execute, which may inhibit the performance of the page, the medicine is worse than the bite. ;)

What this POC raises are the places where we can make significant impacts whereas it is currently buried with prop drilling. One example is querying ALL the rules so that we can use them to build table filters and muted rule counts . The only data that can change as a result of user action is the muted rules count the other data, section and rule number list, can be refs that are not changed between renders because they don't change as a result of use interaction.

@seanrathier

This is a large context, however, this is mostly what the RulesContainer component is doing today with a few minor changes...

My main concern is that this example might not be a good one for the prop drill problem. You have almost all the components on the same level with some exceptions like SearchQuery

<RulesProvider>
        <RulesCounters />
        <EuiSpacer />
        <RulesTableHeader />
        <EuiSpacer />
        <RulesTable selectedRuleId={params.ruleId} onRuleClick={navToRuleFlyout} />
        <RuleFlyout onClose={navToRulePage} />
</RulesProvider>

It looks very clean, but for me it feels somehow off to share the logic and state like that through huge top level context.

When you say rerender, are we talking about rendering a virtual DOM or the page?

I'm talking about components rerendering when smth in the context changes even of these components do not rely on the piece that changes. For example RuleFlyout component right now re-renders whenever smth change in the context, eg. user types in the search. Yes, we have the early exit in this component, but this is a risk that I see - with a huge context it's hard to reason about component rerendering.

But I also understand that "smth feels off for me" and concerns about potential performance problems are NOT good arguments, so I wouldn't stand in the way of such refactoring if the team sees value in it

As I said before, I'm not the most suitable person to chime in on this discussion since I don't know the underlying implementation details.

@maxcold said:

I'm talking about components rerendering when smth in the context changes even of these components do not rely on the piece that changes. For example RuleFlyout component right now re-renders whenever smth change in the context, eg. user types in the search. Yes, we have the early exit in this component, but this is a risk that I see - with a huge context it's hard to reason about component rerendering.

This is something I'm a little concerned with, though. If a component re-renders because something unrelated to it changed in the context's state, then that sounds like a performance regression and we should avoid it. I honestly haven't tested this myself locally so please @seanrathier correct me if I misunderstood.

Also, on the one hand, I agree with @maxcold that, in this particular case, we might not need to move the state from the parent to a context since the component sub-tree is so simple. But there's not really a "right" answer here, it's a matter of preference.

On the other hand, I definitely see some value on keeping part of the changes like turning async callbacks into sync ones, or actually fixing the effect to re-run it when all its dependencies change.

Would be great if more people contribute to the discussion @elastic/kibana-cloud-security-posture. @seanrathier has had this PR open for 3 weeks and even though this has low-prio, we should make a decision at some point. Whether we ship this or not, it'd be great to provide some feedback and agree as a team on certain patterns/styles :)

Lol, as often happens I missed NOT where it mattered :)

But I also understand that "smth feels off for me" and concerns about potential performance problems are NOT good arguments, so I wouldn't stand in the way of such refactoring if the team sees value in it